Skip to content

Add reverseBits operations to Bytes.sol #5724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jul 10, 2025

Conversation

ernestognw
Copy link
Member

@ernestognw ernestognw commented Jun 9, 2025

Needed for #5680

PR Checklist

  • Tests
  • Documentation
  • Changeset entry (run npx changeset add)

Copy link

changeset-bot bot commented Jun 9, 2025

🦋 Changeset detected

Latest commit: fff6ad2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openzeppelin-solidity Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@ernestognw ernestognw changed the title Add operations to Math.sol Add reverseBits operations to Math.sol Jun 9, 2025
@ernestognw ernestognw mentioned this pull request Jun 9, 2025
3 tasks
@ernestognw ernestognw added this to the 5.5 milestone Jun 9, 2025
@Amxx
Copy link
Collaborator

Amxx commented Jun 10, 2025

Should that be in Math, or should we start a Byte.sol file for this, and CLZ (and other) ?

It doesn't feel like this is a math operation strictly speaking.

@ernestognw ernestognw requested a review from a team as a code owner July 9, 2025 17:04
@ernestognw ernestognw changed the title Add reverseBits operations to Math.sol Add reverseBits operations to Bytes.sol Jul 9, 2025
@ernestognw
Copy link
Member Author

Should that be in Math, or should we start a Byte.sol file for this, and CLZ (and other) ?

It doesn't feel like this is a math operation strictly speaking.

After the review and changes made I think it makes sense to add it to the Bytes.sol library so I updated accordingly. wdyt?

@ernestognw ernestognw requested a review from Amxx July 9, 2025 17:41
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
@Amxx
Copy link
Collaborator

Amxx commented Jul 9, 2025

For the record, I tried seeing is assembly would save gas:

    function reverseBits256(bytes32 value) internal pure returns (bytes32) {
        assembly ("memory-safe") {
            let mask
            // swap bytes
            mask := mul(div(not(0), 0xFFFF), 0xFF)
            value := or(and(shr(8, value), mask), shl(8, and(value, mask)))
            // swap 2-bytes long pairs
            mask := mul(div(not(0), 0xFFFFFFFF), 0xFFFF)
            value := or(and(shr(16, value), mask), shl(16, and(value, mask)))
            // swap 4-bytes long pairs
            mask := mul(div(not(0), 0xFFFFFFFFFFFFFFFF), 0xFFFFFFFF)
            value := or(and(shr(32, value), mask), shl(32, and(value, mask)))
            // swap 8-bytes long pairs
            mask := mul(div(not(0), 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF), 0xFFFFFFFFFFFFFFFF)
            value := or(and(shr(64, value), mask), shl(64, and(value, mask)))
            // swap 16-bytes long pairs
            value := or(shr(128, value), shl(128, value))
        }
        return value;
    }

It doesn't !

@Amxx
Copy link
Collaborator

Amxx commented Jul 9, 2025

It all looks good to me, except the name of the functions.

Lets take the example of reverseBits16. The name sugest it reverses the 16 bits that compose the object. So I would expect that it turn the binary 0b0000000000000001 (0x0001) into 0b1000000000000000 (0x8000) ... but the output is 0b0000000100000000 (0x0100).

what it reverses are not the inidividual bits, its the bytes.

So IMO we should either:

  • change the behavior to reverse at the bit level
  • change the name from reverseBits16 to reverseBytes2

@ernestognw ernestognw requested a review from Amxx July 9, 2025 20:16
Copy link
Collaborator

@Amxx Amxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, most of the unit tests are not really necessary, and the fuzzing covers its already.
But that is definitelly not a merge blocker.

LGTM.

@ernestognw ernestognw merged commit 5def3f7 into OpenZeppelin:master Jul 10, 2025
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants